feat(observability): give every cache surface one vocabulary for a hit - #10244
Conversation
Eight cache-like surfaces each report avoidance under their own event name, in three different vocabularies -- *_cache_hit, *_reuse and *_one_shot_skip -- and no single view knows all of them. So the obvious question is answered wrongly by the obvious query. Measured on the Orb over 24h: asking %cache_hit% vs %cache_miss% reports the AI review cache at 0.44% (1 hit, 228 misses) when its real rate is 78.1%, because the 811 avoided runs live in ai_review_one_shot_reuse and ai_review_frozen_reuse, which contain neither the word 'cache' nor the word 'hit'. A 177x understatement, and the kind that sends someone optimising a cache that already works. Maps each of the 20 events to a shared (cache_surface, cache_outcome) pair stamped into its audit metadata. Event names are untouched, so every existing dashboard, alert and query keeps working, and one query now aggregates all of them. Stamped centrally in recordAuditEvent, NOT at the ~20 call sites: a field each call site must remember to add is a field a future call site forgets, which is how the vocabularies diverged in the first place. The call sites are not involved at all. A value a call site sets explicitly always wins, so the stamping is never lossy. An exhaustiveness guard scans src/** for events matching the three vocabularies and fails if any is unregistered, plus the reverse direction for events deleted from the code but left registered. A ninth surface cannot be added without classifying it. grounding is registered but flagged not comparable: it keys on (repo, path, head_sha) and fetches the files the PR CHANGED, whose content differs at every head SHA by construction, so its only possible hit is the same file grounded twice at the same commit. Its rate tracks re-evaluation churn, not cache health -- it fell from 27-61% to ~0% because same-SHA re-evaluation was deliberately driven down. Re-keying on blob content would collapse 1146 rows to 1047, only 8.6% reuse, so there is no fix to apply either. Full analysis on the issue. Closes #10208
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-31 14:24:55 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10244 +/- ##
==========================================
- Coverage 92.25% 91.39% -0.87%
==========================================
Files 938 939 +1
Lines 114694 114700 +6
Branches 27693 27694 +1
==========================================
- Hits 105813 104825 -988
- Misses 7575 8764 +1189
+ Partials 1306 1111 -195
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
1 participant
Summary
The obvious question about caching is answered wrongly by the obvious query.
Eight cache-like surfaces each report avoidance under their own event name, in three different vocabularies —
*_cache_hit,*_reuse, and*_one_shot_skip. No single view knows all of them. Measured on the Orb over 24h, asking%cache_hit%vs%cache_miss%for the AI review cache gives:github_app.ai_review_cache_hitgithub_app.ai_review_cache_missThat reads as a completely dead cache. It is not. The durable cache is bypassed by design for dynamic-context repos (the
featurescomment insrc/review/ai-review-cache-input.ts), and reuse is served by the#regate-churncooldown, which emits different events entirely:github_app.ai_review_one_shot_reusegithub_app.ai_review_frozen_reuseA 177x understatement — and exactly the kind that sends someone optimising a cache that already works.
Approach
Each of the 20 events maps to a shared
(cache_surface, cache_outcome)pair, stamped into its audit metadata. Event names are untouched, so every existing dashboard, alert and query keeps working, and one query now aggregates all of them:Stamped centrally in
recordAuditEvent, not at the ~20 call sites. A field each call site must remember to add is a field a future call site forgets — which is how three vocabularies appeared in the first place. The call sites are not touched at all; the classification happens once, where every one of them already passes through. Same reasoning as #10127 and #10200: make the omission unrepresentable rather than document it. A value a call site sets explicitly always wins, so the stamping is never lossy.An exhaustiveness guard keeps it honest. One test scans
src/**for events matching the three vocabularies and fails if any is unregistered; a second runs the reverse, failing on an event deleted from the code but left registered. A ninth surface cannot be added without being classified, and the registry cannot quietly start lying about what the aggregate covers.cacheOutcomeMetadatadeliberately returnsundefinedfor an unregistered event rather than inferring from its name — inferring would make the guard unfalsifiable.groundingis registered but flagged not comparableHalf (b) of the issue asked whether
grounding's 15.9% meant a broken key or a genuinely uncacheable surface. Measured, it is the second, and it is worse than 15.9% — it is now ~0%:It keys on
(repo, path, head_sha)and fetches the files the PR changed, whose content differs at every head SHA by construction. Its only possible hit is the same file grounded twice at the same commit — so its rate tracks re-evaluation churn, not cache health. It fell because same-SHA re-evaluation was deliberately driven down; it is falling because the system got better.Re-keying on blob content — the obvious fix — was also measured and rejected: 1146 rows keyed by commit collapse to only 1047 keyed by content, 8.6% reuse. There is no fix to apply. So it is registered (one query still sees every surface) and carries a note so nobody reads it as a peer of the fingerprint-keyed caches or tries to optimise it.
#10204 was checked as a candidate cause and ruled out: it merged at 12:34Z, the collapse began at 01:00Z.
Closes #10208
Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changednpm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateDetail:
src/services/cache-outcome.tsis 100% lines and branches (5/5, 2/2), and every changed line inrecordAuditEventis covered with all six branches taken — the two-by-two of cache/non-cache × metadata/none, plus the explicit-override case.db:migrations:check,db:schema-drift:check,selfhost:env-reference:check,docs:drift-check,coverage-boltons:check,dead-exports:check,dead-source-files:check,manifest:drift-check,import-specifiers:check,checkers-wired:check.npm auditreports only pre-existing advisories transitive underrelease-please; this PR changes no dependencies.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails.The added fields are two fixed enum-valued strings derived solely from the event name — no repo, PR, user, path or content data enters the metadata bag by this path, and nothing already in it is removed or overwritten.
UI Evidence
Not applicable — no visible UI, frontend, docs, or extension change.
Notes
This is half (a) of #10208, plus the measurement that resolves half (b). It deliberately does not rename any event or touch
orb-collector.ts's existingAI_REVIEW_REUSE_EVENT_TYPEShand-maintained list — that list is now redundant with the registry and can be collapsed onto it, but doing so changes a live exported metric and belongs in its own change with its own before/after.